Add option to generate a presigned url with a expiration time #117

danielsanfr · 2020-11-17T03:50:11Z

Hello people.

This PR solves the issue: #78

Let me know if everything is right or I need to do something else to accept this PR.

Unfortunately I can't test at DigitalOcean Spaces.

Signed-off-by: Daniel San <[email protected]>

ghost · 2020-11-17T03:50:15Z

Danger run resulted in 1 warning; to find out more, see the checks page.

Generated by 🚫 dangerJS

Signed-off-by: Daniel San <[email protected]>

index.js

Signed-off-by: Daniel San <[email protected]>

davimacedo

It looks good to me. @mtrezza any additional comment?

mtrezza

Some minor requests.

Also, can you please add test cases?

README.md

index.js

Signed-off-by: Daniel San <[email protected]>

danielsanfr · 2020-11-20T03:04:31Z

@mtrezza I did 2 tests 😉 .

danielsanfr · 2020-11-24T16:50:01Z

Hello people.

Let me know if I need to do anything else so that you can merge this PR.

davimacedo · 2020-11-24T17:06:40Z

I think we can merge. @mtrezza do you think it is good now?

mtrezza · 2020-11-24T17:38:29Z

Just my comment on the parameters table thread.

mtrezza · 2020-11-24T19:24:01Z

I have added the parameter to the table and and improved the descriptions.

Now I wonder if we should add an additional test that ensures that the presigned URL only presigns for a getObject operation. Given the implications of presigned URLs, if there is a change in the code the presigned URL could give wide access to the S3 bucket, as described in the docs.

What's your opinion on that?

danielsanfr · 2020-11-26T02:46:38Z

This can really be a problem.

I tried to generate a URL without passing a key in the hope of gaining access to the entire bucket, but the AWS SDK throws an exception. I also tried to put only one prefix as a key to try to access an entire "folder" from within the bucket, but when I open the URL the answer is that the key does not exist.

So, in case you read the bucket or folder, I don't see it as a problem.

Now for the case of writing to a file (or overwriting, via putObject operation), according to the documentation it can be a problem, but I don't know if I did the tests correctly locally, but the generated URL has no "visible" information that references the operation it allows.

That said, I may be mistaken, but I only see 2 "options" (that can be done together):

Put a comment in the code saying that this operation should not be changed within this method;
Create a variable or constant and create a test to validate that it has the value (getObject);

They are just suggestions, let me know if something really needs to be done or if we should make my suggestions or if you have any other ideas?

mtrezza · 2020-11-26T10:53:22Z

My two concerns are:

the Parse Server S3 Adapter code is changed and getObject is not set as allowed operation by mistake
the AWS SDK has a breaking change in the future and after upgrade the operations parameter is ignored and needs to be supplied in a different way

To address this I suggest to:

add a comment in our code
add a test case to spy on a method deep enough in the AWS SDK, preferably after some validation if there is any, ensuring that it receives getObject as allowed operation

danielsanfr · 2020-12-04T03:55:41Z

Hello.

Could you add the comment to the code?

Regarding the test, let me know if this test is enough for you: danielsanfr@8266a66 . If so, I'll make the git cherry-pick.

mtrezza · 2020-12-04T04:02:48Z

The operation check should be a separate test case with a description that explains specifically what it is testing, to give this more significance.

Also, can you spy on a method within the S3 adapter and just call through or mock it?

The comment can just be a simple one that explains why the operation is specified and removing it can provide broad access to the S3 bucket, maybe with a like to the AWS S3 docs security section where this is explained.

Signed-off-by: Daniel San <[email protected]>

…peration in the getSignedUrl function Signed-off-by: Daniel San <[email protected]>

Signed-off-by: Daniel San <[email protected]>

danielsanfr · 2021-02-02T12:47:58Z

Hello guys. I hope everyone is okay.

@mtrezza, I separated the test for the operation used and added the comment about security concerns in the code. I don't know how to "spy" on a method inside the S3 library, and I sincerely believe that it is not necessary.

I wonder if you can approve this PR or we still need to make more changes?

mtrezza · 2021-07-26T20:13:15Z

@danielsanfr Apologies for the long wait time, I have overlooked this in my notifications.

mtrezza · 2021-07-26T20:20:09Z

spec/test.spec.js

-        directAccess: true,
-        bucketPrefix: 'foo/bar/',
-        baseUrl: 'http://example.com/files',
+    ['http://example.com/files', () => 'http://example.com/files'].forEach((baseUrl) => {


Could you please refactor this for simplicity. We normally do not use such a pattern anywhere where we wrap a whole list of tests into a forEach, so this may cause confusion or test execution issues, e.g. with test randomization.

Then this should be good to merge.

dblythy · 2021-08-24T06:48:08Z

@danielsanfr I have been using this branch on my project, and it's working well. Thanks for your efforts!!

mtrezza · 2021-08-24T07:44:59Z

@danielsanfr Could you address the open questions so we can merge this? I think there is just some minor refactoring left.

mtrezza · 2022-08-29T12:11:30Z

@danielsanfr @dblythy Could anyone do this minor refactor? Then we can merge this feature and make is available officially. And the PR needs a rebase I think.

andrewalc · 2023-03-27T15:47:52Z

any updates on this? we really need this since its an important security feature for file downloads.

mtrezza · 2023-03-27T19:14:17Z

Would you want to create a new PR based on this PR and address the changes mentioned above?

andrewalc · 2023-03-29T20:06:45Z

Just opened a PR here #180. I branched off this and refactored the one test you mentioned, let me know if i need to change anything else to get this through 👍

Add option to generate a presigned url with a expiration time

642feac

Signed-off-by: Daniel San <[email protected]>

Update README.md

6495047

Signed-off-by: Daniel San <[email protected]>

danielsanfr marked this pull request as ready for review November 17, 2020 04:01

danielsanfr mentioned this pull request Nov 17, 2020

Wrong options type when set from environment variables #118

Open

davimacedo reviewed Nov 17, 2020

View reviewed changes

index.js Outdated Show resolved Hide resolved

Uses AWS S3 presigned url together with baseUrl

ea46045

Signed-off-by: Daniel San <[email protected]>

danielsanfr force-pushed the feature/presigned-url branch from 3fb8650 to ea46045 Compare November 19, 2020 00:30

Improves code to decrease complexity

1c4948c

Signed-off-by: Daniel San <[email protected]>

davimacedo previously approved these changes Nov 19, 2020

View reviewed changes

mtrezza requested changes Nov 19, 2020

View reviewed changes

README.md Outdated Show resolved Hide resolved

index.js Outdated Show resolved Hide resolved

danielsanfr added 7 commits November 19, 2020 21:32

Simplify code complexity using "function early return" concept

1f82746

Signed-off-by: Daniel San <[email protected]>

Update README.md

e901d51

Signed-off-by: Daniel San <[email protected]>

Fix get presingedUrl and presignedUrl from class constructor args

1051f98

Signed-off-by: Daniel San <[email protected]>

Using the AWS S3 default expire time

9718934

Signed-off-by: Daniel San <[email protected]>

When using s3client.getSignedUrl(), avoid return old path-style URL

140bdf9

Signed-off-by: Daniel San <[email protected]>

Remove redundant code

7a3c5ca

Signed-off-by: Daniel San <[email protected]>

Add tests for presigned url

21e4d9c

Signed-off-by: Daniel San <[email protected]>

added new parameters to parameter table

d761881

Small test improvement

4b5dd8d

Signed-off-by: Daniel San <[email protected]>

davimacedo requested a review from mtrezza December 11, 2020 22:50

danielsanfr added 2 commits February 2, 2021 09:20

Add test to ensure that the AWS S3 client receives getObject as the o…

2330ac6

…peration in the getSignedUrl function Signed-off-by: Daniel San <[email protected]>

Add comment to explain security concerns when using presigned URL

0873698

Signed-off-by: Daniel San <[email protected]>

danielsanfr force-pushed the feature/presigned-url branch from 8edfec0 to 0873698 Compare February 2, 2021 12:41

danielsanfr mentioned this pull request Feb 2, 2021

Returning wrong file name #140

Open

mtrezza requested changes Jul 26, 2021

View reviewed changes

dblythy mentioned this pull request Oct 24, 2022

Restrict file download parse-community/parse-server#8241

Open

3 tasks

andrewalc mentioned this pull request Mar 29, 2023

feat: Add option to generate pre-signed URL with expiration time #180

Merged

mtrezza dismissed davimacedo’s stale review via 0873698 April 21, 2023 13:48

danielsanfr closed this May 17, 2023

danielsanfr deleted the feature/presigned-url branch May 17, 2023 03:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to generate a presigned url with a expiration time #117

Add option to generate a presigned url with a expiration time #117

danielsanfr commented Nov 17, 2020 •

edited

Loading

ghost commented Nov 17, 2020 •

edited by ghost

Loading

davimacedo left a comment

mtrezza left a comment

danielsanfr commented Nov 20, 2020

danielsanfr commented Nov 24, 2020

davimacedo commented Nov 24, 2020

mtrezza commented Nov 24, 2020

mtrezza commented Nov 24, 2020 •

edited

Loading

danielsanfr commented Nov 26, 2020

mtrezza commented Nov 26, 2020 •

edited

Loading

danielsanfr commented Dec 4, 2020

mtrezza commented Dec 4, 2020 •

edited

Loading

danielsanfr commented Feb 2, 2021

mtrezza commented Jul 26, 2021

mtrezza Jul 26, 2021 •

edited

Loading

dblythy commented Aug 24, 2021

mtrezza commented Aug 24, 2021 •

edited

Loading

mtrezza commented Aug 29, 2022 •

edited

Loading

andrewalc commented Mar 27, 2023

mtrezza commented Mar 27, 2023

andrewalc commented Mar 29, 2023

Add option to generate a presigned url with a expiration time #117

Add option to generate a presigned url with a expiration time #117

Conversation

danielsanfr commented Nov 17, 2020 • edited Loading

ghost commented Nov 17, 2020 • edited by ghost Loading

davimacedo left a comment

Choose a reason for hiding this comment

mtrezza left a comment

Choose a reason for hiding this comment

danielsanfr commented Nov 20, 2020

danielsanfr commented Nov 24, 2020

davimacedo commented Nov 24, 2020

mtrezza commented Nov 24, 2020

mtrezza commented Nov 24, 2020 • edited Loading

danielsanfr commented Nov 26, 2020

mtrezza commented Nov 26, 2020 • edited Loading

danielsanfr commented Dec 4, 2020

mtrezza commented Dec 4, 2020 • edited Loading

danielsanfr commented Feb 2, 2021

mtrezza commented Jul 26, 2021

mtrezza Jul 26, 2021 • edited Loading

Choose a reason for hiding this comment

dblythy commented Aug 24, 2021

mtrezza commented Aug 24, 2021 • edited Loading

mtrezza commented Aug 29, 2022 • edited Loading

andrewalc commented Mar 27, 2023

mtrezza commented Mar 27, 2023

andrewalc commented Mar 29, 2023

danielsanfr commented Nov 17, 2020 •

edited

Loading

ghost commented Nov 17, 2020 •

edited by ghost

Loading

mtrezza commented Nov 24, 2020 •

edited

Loading

mtrezza commented Nov 26, 2020 •

edited

Loading

mtrezza commented Dec 4, 2020 •

edited

Loading

mtrezza Jul 26, 2021 •

edited

Loading

mtrezza commented Aug 24, 2021 •

edited

Loading

mtrezza commented Aug 29, 2022 •

edited

Loading